Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the inconsistency handling between infra image and normal task image #9580

Merged
merged 3 commits into from
Jan 22, 2021

Conversation

fredwangwang
Copy link
Contributor

fix a minor inconsistency in how nomad handles image url for task and infra.

basically a duplication of

// Remove any http
if strings.HasPrefix(driverConfig.Image, "https://") {
driverConfig.Image = strings.Replace(driverConfig.Image, "https://", "", 1)
}

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 9, 2020

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 9, 2020 03:12 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 9, 2020 03:12 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 9, 2020 03:33 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 9, 2020 03:33 Inactive
tgross
tgross previously requested changes Dec 9, 2020
// Remove any http
if strings.HasPrefix(d.config.InfraImage, "https://") {
d.config.InfraImage = strings.Replace(d.config.InfraImage, "https://", "", 1)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We perform this operation on the main image just before starting the task. It feels like it'd make sense to either:

  • Do this in driver/docker/network.go where we pull the infra image.
  • Do both the main image and infra image in this spot.

Copy link
Contributor Author

@fredwangwang fredwangwang Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repo, _ := parseDockerImage(d.config.InfraImage)

I actually tried to put it in network.go at beginning haha. But quite some places read *d.config.InfraImage directly, so if I want those methods to take the change, I need to mutate global struct state. If I mutate the global state, if doesn't feel right to do it every time before pull infra. So I put it here.

@tgross could you help to put it where you think fits most? It's a minor inconsistency, which was causing some confusion to us, but def not a blocker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I mutate the global state, if doesn't feel right to do it every time before pull infra.

Yeah, I'm inclined to agree with you there. I know we had a couple PRs recently around the infrastructure images, so I want to make sure I get the opinion of folks who worked on that. @shoenig what do you think about moving the http:// stripping for the infra images here too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. I do feel that both code blocks should be using https://golang.org/pkg/strings/#TrimPrefix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. @fredwangwang sorry about the delay in getting back on this but do you think you can make that change? If not, happy to carry this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go for it @tgross, and thanks for getting back to this issue!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that those two sections of code didn't both have access to the same variables, so it doesn't make sense to handle them in the same place after all. I've made @shoenig's suggestion to use TrimPrefix, rebased this PR on master, and added a changelog entry.

@tgross tgross force-pushed the inconsistency-image-handling branch from ee8b742 to 0434b9e Compare January 22, 2021 13:39
@vercel vercel bot temporarily deployed to Preview – nomad January 22, 2021 13:39 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 22, 2021 13:39 Inactive
@tgross tgross dismissed their stale review January 22, 2021 13:39

I've carried the PR

@tgross tgross requested a review from krishicks January 22, 2021 13:41
@tgross tgross merged commit cf3f8eb into hashicorp:master Jan 22, 2021
@tgross
Copy link
Member

tgross commented Jan 22, 2021

Bah, I meant to squash that. But this is merged and will ship in the next normal patch release.

@tgross tgross added this to the 1.0.3 milestone Jan 22, 2021
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants